Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! このプルリクエストは、自動車Entityの実装を継続し、特にそのクライアントサイドでのレンダリング機能に焦点を当てています。VertexConsumerを直接利用してテスト用の外観を設定し、Entityの基本的な動作とモデル構造を確立しました。これにより、今後の本格的なモデル統合に向けた基盤が構築されています。 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new CarModel and CarRenderer for a CarEntity, defining its 3D model and rendering logic. The CarEntity now has a defined size and updated Javadoc comments. However, several critical issues were identified: the CarRenderer's buildQuad method uses incorrect and unnormalized normal vectors, leading to improper lighting, and the CarEntity's movement logic does not account for its orientation, causing it to always move along the world's Z-axis. Additionally, the CarModel's texture size is too small for its texture offsets, potentially causing rendering artifacts. The reviewer also suggested removing commented-out code for better maintainability and simplifying the normal vector calculation in CarRenderer.
src/main/java/com/portofino/polygontrainmod/client/renderer/CarRenderer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/portofino/polygontrainmod/client/model/CarModel.java
Outdated
Show resolved
Hide resolved
src/main/java/com/portofino/polygontrainmod/client/renderer/CarRenderer.java
Outdated
Show resolved
Hide resolved
src/main/java/com/portofino/polygontrainmod/PolygonTrainModClient.java
Outdated
Show resolved
Hide resolved
src/main/java/com/portofino/polygontrainmod/client/renderer/CarRenderer.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
| this.setYRot(this.getYRot() + turn); | ||
| this.setYRot(this.getYRot() - turn); | ||
| // TODO: 旋回に対応 | ||
| this.setDeltaMovement(this.getDeltaMovement().x, this.getDeltaMovement().y, -forward); |
There was a problem hiding this comment.
現在の実装では、setDeltaMovementのz成分に-forwardを直接設定しているため、自動車は向きに関わらず常にワールド座標のZ軸方向にしか移動しません。TODOコメントで旋回への対応が示されていますが、これは車両の挙動として重大なバグです。車両の向き(Y-Rot)を考慮して移動ベクトルを計算する必要があります。
| this.setDeltaMovement(this.getDeltaMovement().x, this.getDeltaMovement().y, -forward); | |
| float yawRad = this.getYRot() * ((float)Math.PI / 180F); | |
| double dx = -Mth.sin(yawRad) * forward; | |
| double dz = Mth.cos(yawRad) * forward; | |
| this.setDeltaMovement(dx, this.getDeltaMovement().y, dz); |
| } | ||
|
|
||
| private void buildQuad(VertexConsumer buffer, Matrix4f matrix, int light) { | ||
| Vector3f testNormalVector = new Vector3f(1, 0, 1).normalize(); |
| buffer.addVertex(matrix, 0, 1, 0).setColor(255, 255, 255, 255).setUv(0, 0).setOverlay(OverlayTexture.NO_OVERLAY).setLight(light).setNormal(0, 0, 1); | ||
| buffer.addVertex(matrix, 0, 0, 0).setColor(255, 255, 255, 255).setUv(0, 1).setOverlay(OverlayTexture.NO_OVERLAY).setLight(light).setNormal(0, 0, 1); | ||
| buffer.addVertex(matrix, 1, 0, 0).setColor(255, 255, 255, 255).setUv(1, 1).setOverlay(OverlayTexture.NO_OVERLAY).setLight(light).setNormal(x, y, z); | ||
| buffer.addVertex(matrix, 1, 1, 0).setColor(255, 255, 255, 255).setUv(1, 1).setOverlay(OverlayTexture.NO_OVERLAY).setLight(light).setNormal(x, y, z); | ||
|
|
||
| // 両ポリゴンの重なる2頂点について、ナナメ45度の方向の法線ベクトルを追加して、簡易的なスムースシェーディング処理を行う | ||
|
|
||
| // ポリゴン2 | ||
| buffer.addVertex(matrix, 1, 1, 0).setColor(255, 255, 255, 255).setUv(0, 0).setOverlay(OverlayTexture.NO_OVERLAY).setLight(light).setNormal(x, y, z); | ||
| buffer.addVertex(matrix, 1, 0, 0).setColor(255, 255, 255, 255).setUv(0, 1).setOverlay(OverlayTexture.NO_OVERLAY).setLight(light).setNormal(x, y, z); | ||
| buffer.addVertex(matrix, 1, 0, -1).setColor(255, 255, 255, 255).setUv(1, 1).setOverlay(OverlayTexture.NO_OVERLAY).setLight(light).setNormal(1, 0, 0); | ||
| buffer.addVertex(matrix, 1, 1, -1).setColor(255, 255, 255, 255).setUv(1, 0).setOverlay(OverlayTexture.NO_OVERLAY).setLight(light).setNormal(1, 0, 0); |
There was a problem hiding this comment.
buildQuadメソッド内の頂点追加コードは、.setColor(...), .setOverlay(...), .setLight(...) の部分が何度も繰り返されています。コードの可読性と保守性を向上させるために、これらの共通部分をカプセル化するプライベートヘルパーメソッドを作成することを検討してください。
例えば、以下のようなヘルパーメソッドを定義できます。
private void addVertexWithDefaultParams(VertexConsumer buffer, Matrix4f matrix, float x, float y, float z, float u, float v, int light, float nx, float ny, float nz) {
buffer.addVertex(matrix, x, y, z)
.setColor(255, 255, 255, 255)
.setUv(u, v)
.setOverlay(OverlayTexture.NO_OVERLAY)
.setLight(light)
.setNormal(nx, ny, nz);
}これにより、buildQuadメソッドの記述が簡潔になります。
| buffer.addVertex(matrix, 0, 1, 0).setColor(255, 255, 255, 255).setUv(0, 0).setOverlay(OverlayTexture.NO_OVERLAY).setLight(light).setNormal(0, 0, 1); | ||
| buffer.addVertex(matrix, 0, 0, 0).setColor(255, 255, 255, 255).setUv(0, 1).setOverlay(OverlayTexture.NO_OVERLAY).setLight(light).setNormal(0, 0, 1); | ||
| buffer.addVertex(matrix, 1, 0, 0).setColor(255, 255, 255, 255).setUv(1, 1).setOverlay(OverlayTexture.NO_OVERLAY).setLight(light).setNormal(x, y, z); | ||
| buffer.addVertex(matrix, 1, 1, 0).setColor(255, 255, 255, 255).setUv(1, 1).setOverlay(OverlayTexture.NO_OVERLAY).setLight(light).setNormal(x, y, z); |
There was a problem hiding this comment.
ポリゴン1の4番目の頂点のUV座標が (1, 1) に設定されていますが、これは3番目の頂点と同じです。これによりテクスチャが歪んで表示される可能性があります。四角形を正しくマッピングするには、UV座標を (1, 0) に変更する必要があると思われます。
| buffer.addVertex(matrix, 1, 1, 0).setColor(255, 255, 255, 255).setUv(1, 1).setOverlay(OverlayTexture.NO_OVERLAY).setLight(light).setNormal(x, y, z); | |
| buffer.addVertex(matrix, 1, 1, 0).setColor(255, 255, 255, 255).setUv(1, 0).setOverlay(OverlayTexture.NO_OVERLAY).setLight(light).setNormal(x, y, z); |
最低限の動作と、テスト用の外見を設定
VertexConsumerのテストのために、ポリゴン二枚だけの外見を追加
EntityModelは使用しない予定だが、使用方法のメモのためにコードはコメントのまま保持コメントは削除